Skip to content

tests: add or delete copy/move ctors where needed to make type traits match reality #5833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 8, 2025
Merged

tests: add or delete copy/move ctors where needed to make type traits match reality #5833

merged 1 commit into from
Sep 8, 2025

Conversation

oremanj
Copy link
Contributor

@oremanj oremanj commented Sep 8, 2025

#5800 will attempt to instantiate copy/move constructors for every bound type that appears copy/move constructible. (Currently, only types that are returned from a bound function will try to use these constructors, so if a type is used only in argument position, it's possible that it satisfies is_copy_constructible but actually odr-using the copy constructor would fail. Ditto for moves.) See that PR for more on the rationale.

There are several classes used in pybind11's test suite that satisfy is_copy_constructible, but produce warnings or errors if you try to actually odr-use their (implicitly-defined) copy constructor:

  • In recent C++ versions, use of the implicitly-defaulted copy constructor is deprecated for any class with a nontrivial (including virtual) destructor. This produces several warnings due to the test suite's frequent use of patterns that count how many destructions have occurred.
  • If a type has no copy constructor and none of its immediate members are non-copyable but some of their subobjects are non-copyable, it will look copyable to std type traits, but actually generating the copy constructor produces an error. This is relevant to DataFieldsHolder in test_class_sh_property_non_owning.cpp, which contains a std::vector of move-only type DataField.

This PR adds defaulted or deleted copy constructors for these cases, as semantically appropriate.

(No user-visible impact, so no changelog entry.)

@rwgk
Copy link
Collaborator

rwgk commented Sep 8, 2025

I don't remember seeing this flake before:

CI / 🐍 (macos-latest, 3.14t, -DCMAKE_CXX_STANDARD=20) / 🧪 (pull_request) Failing after 7m

FAILED ../../tests/test_callbacks.py::test_async_callbacks - assert 17 == 22
 +  where 17 = sum([6, 4, 7])
 +  and   22 = sum(<generator object test_async_callbacks.<locals>.<genexpr> at 0x23deaef8f10>)
== 1 failed, 959 passed, 34 skipped, 2 xfailed, 1 xpassed in 75.23s (0:01:15) ==

I'll try a rerun.

@rwgk
Copy link
Collaborator

rwgk commented Sep 8, 2025

Question for my understanding: After #5800 is merged, will general users have to make similar changes? (I think that's fine, but if yes, we shouldn't forget to mention that in the release notes and maybe upgrade guide.)

@oremanj
Copy link
Contributor Author

oremanj commented Sep 8, 2025

After #5800 is merged, will general users have to make similar changes?

Yes. There's some discussion of that in the PR description.

@rwgk
Copy link
Collaborator

rwgk commented Sep 8, 2025

Ugh

CI / 🐍 (macos-latest, 3.14t, -DCMAKE_CXX_STANDARD=20) / 🧪 (pull_request) Cancelled after 29m

Now it was stuck here, which I've seen at least twice before:

Run cmake --build build --target cpptest
  cmake --build build --target cpptest
  shell: /bin/bash -e {0}
  env:
    PYTHONDEVMODE: 1
    PIP_BREAK_SYSTEM_PACKAGES: 1
    PIP_ONLY_BINARY: numpy
    FORCE_COLOR: 3
    PYTEST_TIMEOUT: 300
    VERBOSE: 1
    CMAKE_COLOR_DIAGNOSTICS: 1
    pythonLocation: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
    PKG_CONFIG_PATH: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded/lib/pkgconfig
    Python_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
    Python2_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
    Python3_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
    UV_CACHE_DIR: /Users/runner/work/_temp/setup-uv-cache
    CMAKE_GENERATOR: Ninja
Change Dir: '/Users/runner/work/pybind11/pybind11/build'
Run Build Command(s): /opt/homebrew/bin/ninja -v cpptest
[0/2] /opt/homebrew/bin/cmake -P /Users/runner/work/pybind11/pybind11/build/CMakeFiles/VerifyGlobs.cmake
Error: The operation was canceled.

Trying again...

@rwgk rwgk merged commit 68cbae6 into pybind:master Sep 8, 2025
209 of 211 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 8, 2025
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oremanj @rwgk